-
Notifications
You must be signed in to change notification settings - Fork 3k
Perform hidden file check on relative data file path #4551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks ! LGTM :)
Though it doesn't fix the issue if the relative path points to an actual file (not a pattern) and if it contains a double underscores (so it doesn't completely "Fix #4549"):
resolve_patterns_locally_or_by_urls(
base_path="/Users/quentinlhoest/Desktop/hf/datasets/playground",
patterns=[".yo/data.txt"]
)^ this one should return the file
but we still have to make sure this one doesn't:
resolve_patterns_locally_or_by_urls(
base_path="/Users/quentinlhoest/Desktop/hf/datasets/playground",
patterns=["*/data.txt"]
)(and probably out of scope for now - not important)
maybe this one should return the file
resolve_patterns_locally_or_by_urls(
base_path="/Users/quentinlhoest/Desktop/hf/datasets/playground",
patterns=[".yo/*"]
)|
I'm aware of this behavior, which is tricky to solve due to fsspec's hidden file handling (see #4115 (comment)). I've tested some regex patterns to address this, and they seem to work (will push them on Monday; btw they don't break any of fsspec's tests, so maybe we can contribute this as an enhancement to them). Also, perhaps we should include the files starting with |
|
I see. Feel free to merge this one if it's good for you btw :)
The point was mainly to ignore |
|
Very elegant solution! Feel free to merge if the CI is green after adding the tests. |
|
CI failure is unrelated to this PR |
Fix #4549